-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement full user name to file name conversion #251
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is certainly something, huh.
some thoughts inline: 10^15 is definitely way more checks than could possibly make sense, but whatever.
More importantly, I think constructing the set of paths each time is a mistake; we should just maintain a set of normalized paths, similarly to how we maintain a set of names.
I ended up tackling #253 in the last commit. |
Needs to be rebased on top of #254 if that goes through :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay a few more little comments inline but I think this is close. Thanks!
I noticed that inserting something to escape a reserved name could push the string over 255 characters. Maybe I need to do the escaping before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
One day I must fuzz this 🤔 |
Closes #85
Closes #252
Alright, this took me a week to implement.
Name::is_valid
must checkchars()
instead ofbytes()
, because the spec talks about "Any character with a Unicode value in the ranges [...]". E.g. the name "hello💝" encoded as bytes happens to include a banned control character but is a valid name.user_name_to_file_name
now...&Name
, which guarantees some properties relevant for file name generation alreadyusize
over- or underflow occurs, e.g. by one of name, prefix or suffix being pathologically long. I'm not too confident about it though... I should probably fuzz it?Layer::new
and streamline its callers.user_name_to_file_name
with a set of lowercased strings of existing file names for clash checking.I think clashes are very unlikely, so this does feel a bit excessive... but I also can't rule them out completely. At least, I only try one strategy to resolve clashes instead of the two in the UFO spec example.
There is a discussion at unified-font-object/ufo-spec#164 about a different and better file naming scheme, but so far without a definitive conclusion.
I need to look more closely at
Font::save()
to see how to implement erroring out when a file should be unique but the filesystem thinks differently. ShouldGlyph::save_with_options()
error out if the destination exists already?Also, random observation: do we need to change or regenerate file names when removing/renaming glyphs?Doesn't seem like it.